Skip to content

Add config source interface and environment variable source#5

Open
ubaskota wants to merge 4 commits intoconfig_resolutionfrom
config_source
Open

Add config source interface and environment variable source#5
ubaskota wants to merge 4 commits intoconfig_resolutionfrom
config_source

Conversation

@ubaskota
Copy link
Owner

Issue #, if available:

Description of changes:
This change adds the ConfigSource protocol to smithy-core interfaces and implements EnvironmentSource in smithy-aws-core for resolving configuration values from environment variables. Includes unit tests covering protocol compliance, key-to-env-var mapping, edge cases, and no-caching behavior.

  • Tests *
    Added unit tests to validate the expected behavior. Verified that tests pass.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment on lines +9 to +15
def test_implements_config_source_protocol(self):
source = EnvironmentSource()
assert isinstance(source, ConfigSource)
assert hasattr(source, "name")
assert hasattr(source, "get")
assert callable(source.get)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems like overkill. It should be safe due to typechecking from the resolver, right?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, its type checked when called from the resolver.

assert value1 != value2

def test_env_var_names_are_case_sensative(self):
with patch.dict(os.environ, {"aws_region": "us-west-2"}, clear=False):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not work on systems where the host has case-insensitive environment variables like windows.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. I decided to remove this test as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants